feat: replace HIVE_LEAN_TEST_DRIVER env var with test-driver subcommand#375
feat: replace HIVE_LEAN_TEST_DRIVER env var with test-driver subcommand#375IbrahimIjai wants to merge 1 commit into
Conversation
Greptile SummaryThis PR replaces the implicit
Confidence Score: 5/5Safe to merge — the change is a clean structural refactor that makes the two operating modes explicit and mutually exclusive at the CLI level. Both changed files are touched only to remove dead env-var code and reshape the CLI parser. The test-driver logic itself is unchanged; run_test_driver and run_node are called with equivalent arguments to before. The only downstream action required is updating the hive shim, which the PR explicitly calls out. No files require special attention. The hive client shim (not part of this repo) will need a corresponding update to call
|
| Filename | Overview |
|---|---|
| bin/ethlambda/src/main.rs | Replaces flat CliOptions with Cli + Commands subcommand enum; extracts run_node(); adds TestDriverArgs. Logic is correct — a minor redundant return in the TestDriver match arm is the only style nit. |
| crates/net/rpc/src/test_driver.rs | Removes TEST_DRIVER_ENV constant, test_driver_enabled(), parse_truthy_env_value(), and their associated tests; updates module doc comment. Clean deletion with no remaining references. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ethlambda binary starts] --> B[Init tracing + metrics]
B --> C[clap::parse Cli]
C --> D{cli.command}
D -- "test-driver\n--http-address --api-port --metrics-port" --> E[Build RpcConfig from TestDriverArgs]
E --> F[run_test_driver]
F --> G[start_test_driver_rpc_server\n/lean/v0/test_driver/...]
D -- "node\n--genesis --validators ... all node flags" --> H[run_node]
H --> I[Build RpcConfig from NodeArgs]
I --> J[Load genesis / keys / storage]
J --> K[Spawn BlockChain + P2P actors]
K --> L[start_rpc_server\nnormal consensus API]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
bin/ethlambda/src/main.rs:160-171
The `return` keyword in the `TestDriver` arm is redundant — the `match` is already the tail expression of `main()`, so dropping `return` makes both arms consistent and more idiomatic Rust.
```suggestion
match cli.command {
Commands::TestDriver(args) => {
let rpc_config = RpcConfig {
http_address: args.http_address,
api_port: args.api_port,
metrics_port: args.metrics_port,
};
info!("Booting in test-driver mode");
run_test_driver(rpc_config).await
}
Commands::Node(options) => run_node(*options).await,
}
```
Reviews (1): Last reviewed commit: "feat: replace HIVE_LEAN_TEST_DRIVER env ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| match cli.command { | ||
| Commands::TestDriver(args) => { | ||
| let rpc_config = RpcConfig { | ||
| http_address: args.http_address, | ||
| api_port: args.api_port, | ||
| metrics_port: args.metrics_port, | ||
| }; | ||
| info!("Booting in test-driver mode"); | ||
| return run_test_driver(rpc_config).await; | ||
| } | ||
| Commands::Node(options) => run_node(*options).await, | ||
| } |
There was a problem hiding this comment.
return keyword in the TestDriver arm is redundant — the match is already the tail expression of main(), so dropping return makes both arms consistent and more idiomatic Rust.
| match cli.command { | |
| Commands::TestDriver(args) => { | |
| let rpc_config = RpcConfig { | |
| http_address: args.http_address, | |
| api_port: args.api_port, | |
| metrics_port: args.metrics_port, | |
| }; | |
| info!("Booting in test-driver mode"); | |
| return run_test_driver(rpc_config).await; | |
| } | |
| Commands::Node(options) => run_node(*options).await, | |
| } | |
| match cli.command { | |
| Commands::TestDriver(args) => { | |
| let rpc_config = RpcConfig { | |
| http_address: args.http_address, | |
| api_port: args.api_port, | |
| metrics_port: args.metrics_port, | |
| }; | |
| info!("Booting in test-driver mode"); | |
| run_test_driver(rpc_config).await | |
| } | |
| Commands::Node(options) => run_node(*options).await, | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 160-171
Comment:
The `return` keyword in the `TestDriver` arm is redundant — the `match` is already the tail expression of `main()`, so dropping `return` makes both arms consistent and more idiomatic Rust.
```suggestion
match cli.command {
Commands::TestDriver(args) => {
let rpc_config = RpcConfig {
http_address: args.http_address,
api_port: args.api_port,
metrics_port: args.metrics_port,
};
info!("Booting in test-driver mode");
run_test_driver(rpc_config).await
}
Commands::Node(options) => run_node(*options).await,
}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
It would be nice next time to follow the PR description template attached to the GitHub workflow 🚀 |
Thank you sir, let me update |
Done now... just updated the description to follow the template. Would love your feedback when you are open |
@MegaRedHand will make his reviews. No worries! |
3 participants
🗒️ Description / Motivation
The hive test driver was previously activated by setting
HIVE_LEAN_TEST_DRIVER=1in the environment before starting the binary. It was implicit and brittle if the env var was set, the binary would quietly run in test-driver mode and ignore the normal consensus flags, and--helpgave no hint this mode even existed.This PR replaces the env-var mechanism with an explicit
test-driversubcommand, making the two operating modes structurally separate in the CLI rather than diverging silently at runtime:ethlambda node <...all existing flags...>— normal consensus node (unchanged behaviour)ethlambda test-driver [--http-address] [--api-port] [--metrics-port]— hive test driver modeThis solves the discoverability and footgun problem: the mode is now visible in
--help, each mode owns its own argument set, and the two launch paths can't be confused.What Changed
bin/ethlambda/src/main.rs— replaced the flatCliOptionswith aClistruct and aCommandssubcommand enum. Split arguments intoNodeArgsandTestDriverArgsso each mode declares only the flags it needs. Extracted the node startup logic intorun_node()so the two modes have parallel entry points.crates/net/rpc/src/test_driver.rs— removedTEST_DRIVER_ENV,test_driver_enabled(),parse_truthy_env_value(), and their tests. The env-var activation path is gone entirely. Updated the module doc comment to reflect the new entry point.Correctness / Behavior Guarantees
test-driversubcommand callsrun_test_driverwith anRpcConfigbuilt from the same fields (http_address,api_port,metrics_port) that the env-var path used, so the test-driver RPC surface is unchanged.nodesubcommand path is a straight extraction of the previousmainbody intorun_node()— no consensus flags, defaults, or startup order are modified.HIVE_LEAN_TEST_DRIVER=1is no longer recognised. The hive client shim for ethlambda must be updated to invokeethlambda test-driver(passing--http-address 0.0.0.0if it was relying on that default) instead of setting the env var.Tests Added / Run
parse_truthy_env_valueunit tests incrates/net/rpc/src/test_driver.rs(the function they covered no longer exists).cargo run -- node <existing flags>boots a normal consensus node as before.cargo run -- test-driverboots the hive test driver RPC server.cargo run -- --helpshows both subcommands.Commands run:
make fmt
make lint
cargo test --workspace --release
Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleancargo test --workspace --release— all passing